[PAY-1430] "Leaving Audius" Warning for External Links (and createModal helper) #3860
Conversation
packages/web/src/components/leaving-audius-modal/LeavingAudiusModal.module.css
Show resolved
Hide resolved
|
Preview this change https://demo.audius.co/mjp-dms-leaving-audius |
packages/mobile/src/components/leaving-audius-drawer/LeavingAudiusDrawer.tsx
Outdated
Show resolved
Hide resolved
| @@ -312,6 +319,9 @@ export type CommonState = { | |||
| publishPlaylistConfirmationModal: PublishPlaylistConfirmationModalState | |||
| mobileOverflowModal: MobileOverflowModalState | |||
| modals: ModalsState | |||
| modalsWithState: { | |||
There was a problem hiding this comment.
would the plan be to have models just all unified at some point?
There was a problem hiding this comment.
yeah this could use some discussion. We have other modals just toplevel here, so we could do the same and remove this nesting.
could probably also preemptively merge into modals by moving existing modals from ModalName: boolean | 'closing' to ModalName: { isOpen: boolean | 'closing' } 🤔 Should be a straightforward migration, just some types and underlying store structure, all the call sites stay the same
There was a problem hiding this comment.
It would be nice if modal state could vary in shape. It seems like we have some base modal state (isOpen, onClose, etc), and then state we are passing to modals through reducers for the logic specific to the modal. Maybe those things don't have to co-exist?
Anyway yes, worth some discussion. Would like to get the three ways of doing this down to one 😅
There was a problem hiding this comment.
The coexistence was a big part of the motivation behind the createModal helper, but if we don't want that then maybe there's no need for the helper and we go back to separate reducers etc
| @@ -312,6 +319,9 @@ export type CommonState = { | |||
| publishPlaylistConfirmationModal: PublishPlaylistConfirmationModalState | |||
| mobileOverflowModal: MobileOverflowModalState | |||
| modals: ModalsState | |||
| modalsWithState: { | |||
There was a problem hiding this comment.
It would be nice if modal state could vary in shape. It seems like we have some base modal state (isOpen, onClose, etc), and then state we are passing to modals through reducers for the logic specific to the modal. Maybe those things don't have to co-exist?
Anyway yes, worth some discussion. Would like to get the three ways of doing this down to one 😅
packages/web/src/components/leaving-audius-modal/LeavingAudiusModal.module.css
Show resolved
Hide resolved
packages/web/src/components/leaving-audius-modal/LeavingAudiusModal.module.css
Outdated
Show resolved
Hide resolved
packages/web/src/components/leaving-audius-modal/LeavingAudiusModal.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/components/leaving-audius-modal/LeavingAudiusModal.tsx
Outdated
Show resolved
Hide resolved
| text={messages.goBack} | ||
| onClick={handleClose} | ||
| /> | ||
| <HarmonyButton |
There was a problem hiding this comment.
It occurs to me that we could probably use a within this button that has the external link properties set on it so that we don't have to use window.open. I remember there are cases where window.open isn't allowed...
There was a problem hiding this comment.
ooh do tell more, I definitely want to avoid the window.open if possible!
There was a problem hiding this comment.
Just thinking that since <Link /> supports external link behavior, and we need to open the modal anyway, could embed a <Link /> to the desired URL within the button, and then just have the button close the modal onClick.
So essentially, just presenting the user with the link again and asking them to click it a second time :-p
There was a problem hiding this comment.
I like it, but ideally we don't have a link with a button in it or vice versa, but a button as a link
|
Preview this change https://demo.audius.co/mjp-dms-leaving-audius |
[3436c20] [PAY-1701] Fix "Share to DMs" to work through InboxUnavailableModal (#3874) Marcus Pasell [a740243] Add sdk:update-hotfix (#3875) Dylan Jeffers [a25fd19] [C-2759] Make donation link external (#3872) Dylan Jeffers [15f056c] [PAY-1630] Wire up purchase content sagas (#3834) Randy Schott [998d44b] Fix mobile crash on drawer dismiss (#3871) Reed [7d0e0b3] [PAY-1430] "Leaving Audius" Warning for External Links (and createModal helper) (#3860) Marcus Pasell [bee8bd1] Remove .only on upload cypress test (#3869) Raymond Jacobson [4c0b25f] [C-2926] Implement selected values for upload contextual menu fields (#3848) Dylan Jeffers [5773578] Preserve CIDs for track and collection cover arts (#3866) Marcus Pasell [be0d278] [C-2930] Fix extra space after username in tip to unlock modal (#3845) nicoback2 [f5320be] QA-588 Fix collection card profile link (#3853) nicoback2 [360416e] Fix broken playlist fetch via resolve (#3863) Raymond Jacobson [2dc2c29] [PAY-1695] DMs: Entrypoint Analytics (#3862) Marcus Pasell [f80d366] Minor improvements to SEO flow merged in #3859 (#3861) Raymond Jacobson [b99d62f] Add nodes to env for SEO support (#3859) Raymond Jacobson [20476ee] [C-2941] Modify cloudflare worker to pull in SEO data from discovery nodes (#3858) Raymond Jacobson [7f79830] [C-2879] Add validation to single track upload flow (#3855) Kyle Shanks [6f4fc89] [C-2940] Update google analytics tags and fix embed build (#3856) Raymond Jacobson [3469c89] [C-2852 PLAT-1094 PLAT-1093] Add fetch collection by permalink (#3751) Dylan Jeffers
Description
createModalthat sets up the reducer and hook for a modalDragons
Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Tested both internal and external links in chats have the expected behavior. iOS sim and web
How will this change be monitored?
For features that are critical or could fail silently please describe the monitoring/alerting being added.
Feature Flags
Are all new features properly feature flagged? Describe added feature flags.